feat(implement): make OpenSpec mandatory when configured (Phase 0)#13
Conversation
|
@greptile review |
Greptile SummaryThis PR introduces Phase 0 (OpenSpec Context) to the
Confidence Score: 5/5Safe to merge — Phase 0 logic is well-structured, all previously blocking findings are resolved, and version manifests are consistently aligned. All three previously blocking findings (branch ordering, hardcoded openspec/changes/ path, unbound {change-slug} template) have been addressed across successive rounds. MAIN_TREE resolution via git worktree list --porcelain is correct, cp -r correctly copies into the worktree before staging, and the idempotency guard prevents double-copy on restart. No files require special attention; remaining items are non-blocking polish around main-tree cleanup after step 4a and Phase 0 step-numbering cosmetics.
|
| Filename | Overview |
|---|---|
| commands/implement.md | Adds Phase 0 (OpenSpec Context), inserts step 4a for the first-commit guarantee, and renumbers Phase 2-4 steps 5-17; all previous blocking findings are addressed. |
| .claude-plugin/plugin.json | Minor version bump 1.5.0 → 1.6.0; correct semver level for the behavioral change introduced in this PR. |
| .claude-plugin/marketplace.json | Version bumped from 1.4.0 to 1.6.0, catching up with plugin.json/package.json; the intentional skip of 1.5.0 is explained in the PR description. |
| package.json | Minor version bump 1.5.0 → 1.6.0 to match plugin.json; correct semver level for the Phase 0 behavioral change. |
Reviews (5): Last reviewed commit: "fix(implement): bind {change-slug} as $C..." | Re-trigger Greptile
Introduces a new Phase 0 in /make-no-mistakes:implement that runs BEFORE Phase 1 (Setup). When linear-setup.json has openspec.changesPath configured, every issue MUST have a corresponding OpenSpec change (proposal.md + design.md + tasks.md) before implementation begins. Previously, step 4b in Phase 2 read OpenSpec context only "if found", which made the spec-first discipline silently skippable. Since OpenSpec was officially adopted at Dojo Coding on 2026-03-30 (Slack thread C0AE5MKAX7B), that gap re-introduces exactly the failure mode adoption was meant to prevent: implementations diverging from intent because nobody wrote the intent down. Behavior change: - When openspec.changesPath is set AND the issue is non-trivial (>2 files, architectural decisions, or reviewer-flagged risk), the implementer STOPS and creates the OpenSpec change as the first commit on the implementation branch. - Trivial issues (typo fixes, dependency bumps, single-line changes) may opt out with a one-line PR note. - Repos without openspec.changesPath are unaffected — Phase 0 is skipped entirely, preserving back-compat. Bumps version to 1.4.1 across plugin.json, marketplace.json, and package.json. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
9508df5 to
019604f
Compare
|
Rebased onto 1. Chicken-and-egg branch ordering (Issue 1) 2. Hardcoded CHANGES_PATH=$(jq -r '.openspec.changesPath' linear-setup.json)
grep -r "{issue-id}" "$CHANGES_PATH"/*/proposal.md 2>/dev/nullAll subsequent references in steps 2 & 4a use 3. Semver: 1.4.0 → 1.4.1 → 1.6.0 (Issue 3) Conflicts resolved: kept main's updated description text in @greptile review |
…ntially Greptile re-review on PR #13 caught two real defects in the new Phase 0/Phase 1 text: 1. Phase 1 step 4a's git add command used an undefined placeholder "{changes-path}" — the variable established in Phase 0 step 1 is "\$CHANGES_PATH" (set via jq from linear-setup.json). An agent running the step verbatim would stage nothing, producing an empty first commit and silently breaking the spec-first guarantee the entire phase enforces. Step 4a now re-exports CHANGES_PATH inside the worktree (the parent shell's variable doesn't survive the cd into a fresh worktree) and uses "\$CHANGES_PATH/{change-slug}/" in the git add. 2. The original document restarted step numbering at 4 in Phase 2, colliding with Phase 1's existing step 4 + 4a. Cross-phase references like "step 4" became ambiguous. Renumbered the entire chain so steps run sequentially across all four phases: - Phase 1: 1, 2, 3, 4, 4a - Phase 2: 5, 6, 7 - Phase 3: 8, 9, 10, 11, 12 - Phase 4: 13, 14, 15, 16, 17 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed both Greptile findings in 60e2cbb. 1. 2. Step numbering collision
@greptile review |
…list Greptile re-review on PR #13 caught a P1 ambiguity in step 4a: Phase 0 drafts spec files in the main working tree at {main-tree}/\$CHANGES_PATH/ {change-slug}/, but Phase 1 step 3 cd's into a freshly created worktree that does NOT carry over uncommitted files. The previous wording said "move/copy if not already there" without naming the source path — an agent inside the worktree could resolve it incorrectly and end up running git add on a non-existent path, producing a silently empty docs(openspec) commit and defeating the spec-first guarantee. Step 4a now: 1. Re-exports \$CHANGES_PATH inside the worktree (the parent shell's variable doesn't survive the cd). 2. Computes \$MAIN_TREE via git worktree list --porcelain (first entry is always the primary tree, regardless of which worktree we're currently in). 3. Skips the copy when the directory already exists in the worktree (idempotent on retry). 4. Copies "\$MAIN_TREE/\$CHANGES_PATH/{change-slug}" into the worktree before git add — explicit, unambiguous, agent-runnable verbatim. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Round 3 fixes pushed in 553d81a + PR description updated to reflect actual 1.6.0 bump (was misleadingly stating 1.4.0 → 1.4.1). Fix for Issue 1 (step 4a source path) Phase 0 step 3 drafts files in the main working tree at CHANGES_PATH=$(jq -r '.openspec.changesPath' linear-setup.json)
MAIN_TREE=$(git worktree list --porcelain | awk '/^worktree/ {print $2; exit}')
if [ ! -d "$CHANGES_PATH/{change-slug}" ]; then
mkdir -p "$CHANGES_PATH"
cp -r "$MAIN_TREE/$CHANGES_PATH/{change-slug}" "$CHANGES_PATH/"
fi
git add "$CHANGES_PATH/{change-slug}/"
git commit -m "docs(openspec): {change-slug}"Used Fix for Issue 2 (PR description) Updated body to reflect actual @greptile review |
Greptile round-4 caught the same class of defect as round 2 but for a
different placeholder: {change-slug} was a bare template literal in the
step 4a bash block, sitting next to a properly bound \$CHANGES_PATH. An
agent running the block verbatim would execute
git add "\$CHANGES_PATH/{change-slug}/"
which stages a literal path containing the brace string — staging
nothing — and produces a silently empty docs(openspec) commit.
Two paired fixes:
1. Phase 0 step 3 now states the slug naming convention explicitly:
{issue-id-lowercase}-{short-kebab-description} (lowercase ASCII +
hyphens, e.g. doj-3946-atomic-primitives-sprint). Same shape as
the implementation branch so agents and humans converge on the same
directory name across runs.
2. Step 4a bash block now binds CHANGE_SLUG="<change-slug>" at the top
(placeholder agents/humans MUST replace before running) and uses
"\$CHANGE_SLUG" in every file path expression. mkdir/cp/git add/git
commit all reference the bound variable, so verbatim execution
produces a non-empty commit even if the surrounding text is unread.
Documentation-template uses of {change-slug} elsewhere in the file are
left intact — they refer to the literal directory name in prose, not
shell variables, and agents reading prose interpret them correctly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Round 4 fix in d7ea9c9. Same class of defect as round 2 — bare template literal next to a properly-bound shell variable. Two paired changes:
Documentation-template uses of @greptile review |
Summary
The
/make-no-mistakes:implementskill currently treats OpenSpec context as optional in step 4b ("if exists"). Since OpenSpec was officially adopted at Dojo Coding on 2026-03-30 (Slack thread C0AE5MKAX7B), this gap allows implementations to skip the spec by accident — exactly the failure mode adoption was meant to prevent.This PR introduces a new Phase 0: OpenSpec Context that runs before Phase 1 (Setup) and makes spec-first mandatory when
linear-setup.jsonhasopenspec.changesPathconfigured.Behavior change
openspec.changesPathis unset, Phase 0 is skipped entirelyFiles changed
commands/implement.md— new Phase 0 section, Phase 1 step 4a (defers commit until after the worktree exists), Phase 2–4 step renumber so cross-phase references are unambiguous (1, 2, 3, 4, 4a → 5, 6, 7 → 8, 9, 10, 11, 12 → 13, 14, 15, 16, 17).claude-plugin/plugin.json,package.json— version bump 1.5.0 → 1.6.0 (minor — Phase 0 is a behavioral break for agents relying on the old optional behavior).claude-plugin/marketplace.json— version bump 1.4.0 → 1.6.0 (catching up from a pre-existing drift where it had been left at 1.4.0 while plugin.json/package.json were already at 1.5.0; aligns all three at 1.6.0)Greptile fix history
openspec/changes/path, semver patch→minor{changes-path}placeholder undefined → use$CHANGES_PATH; Phase 2 step numbering collision → renumber 1–17 sequentially$MAIN_TREEviagit worktree list --porcelainandcp -r "$MAIN_TREE/$CHANGES_PATH/..."beforegit addTest plan
/make-no-mistakes:implement DOJ-XXXagainst an issue WITHOUT an OpenSpec change in a repo whereopenspec.changesPathis configured → verify skill stops and prompts to create the specopenspec.changesPathis unset → verify Phase 0 is skipped (back-compat)openspec.changesPathisspecs/changes/(non-default) → verify grep resolves correctly and the spec is detected$MAIN_TREE→ first commit on the branch isdocs(openspec): {change-slug}and is non-emptyCreated by Claude Code on behalf of @lapc506.
Generated with Claude Code